-
Notifications
You must be signed in to change notification settings - Fork 195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(DataStore): improve MutationEvent resiliency to interruptions #3492
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #3492 +/- ##
==========================================
+ Coverage 67.90% 68.18% +0.27%
==========================================
Files 1080 1080
Lines 36406 36426 +20
==========================================
+ Hits 24723 24838 +115
+ Misses 11683 11588 -95
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
301ee20
to
b9528c1
Compare
b9528c1
to
4c497cd
Compare
4c497cd
to
1ed99fa
Compare
...lugins/DataStore/Sources/AWSDataStorePlugin/Storage/SQLite/StorageEngineAdapter+SQLite.swift
Outdated
Show resolved
Hide resolved
AmplifyPlugins/DataStore/Sources/AWSDataStorePlugin/Storage/StorageEngine.swift
Outdated
Show resolved
Hide resolved
...MutationSync/AWSMutationDatabaseAdapter/AWSMutationDatabaseAdapter+MutationEventSource.swift
Show resolved
Hide resolved
AmplifyPlugins/DataStore/Sources/AWSDataStorePlugin/Storage/StorageEngine.swift
Show resolved
Hide resolved
86bb64e
to
9fab7e9
Compare
9fab7e9
to
1bf9b84
Compare
Issue #
Continuation from #3259 (comment)
This PR is a two distinct changes that together improves the resiliency of interruptions caused by
DataStore.stop
.DataStore.save
)1. save and syncMutation under transaction
When
DataStore.save()
is called, it will perform a sequence of reads and writes. If this sequence is interrupted byDataStore.stop()
then not all data that would have been written is committed. For example,SQLiteStorageEngineAdapter.save()
will check if the model exists, decide whether to create an Insert/update, perform the write, and query the model and return it. Then,StorageEngine.save()
checks if sync is enabled, and saves the MutationEvent throughsyncMutation(of: savedModel)
. On completion, the results is returned to the caller ofDataStore.save()
. Ref: #3259 (comment)This PR aims to solve this: If the process is interrupted before the MutationEvent is persisted, then the local data will never be synced to AppSync.
The problem causes subsequent issues for models with associations, ie. Comment belongs to a Post. If the post is never synced to AppSync, then saving the comment will result in
The changes in the PR to add the operations under a transaction guarantees that the two writes are done atomically, or otherwise
DataStore.save
will fail. This doesn't allow the local database to be put in a state where the model is committed but the mutation event is not. In the above example, the post in the model tabel and the mutation event table will only exist if both were successful. If we save the comment without the post, then the local DB operation should fail because there is no parent post2. MutationEvent dequeue include inProcess true events
It can be observed in highly concurrent calls to
DataStore.start
,DataStore.stop
,DataStore.save
that it will interrupt the OutgoingMutationQueue process of processing a MutationEvent and deleting it from the queue (removing it from local storage).DataStore.start
, which starts the remote sync engine, should perform a step to move all inProcess MutationEvents back to false, however there's a timing issue that is difficult to pinpoint. OutgoingMutationQueue's query manages to pick up the second MutationEvent in the queue and sends it off, while the first one that is marked as inProcess isn't being processed, likely that process was already interrupted. Ref #3259 (comment)This PR updates the logic to retrieve the head of the MutationEvent queue, instead of just the InProcess false events.
There is no concern over multiple processes picking up the same InProcess true event since OutgoingMutationQueue operates on one MutationEvent at a time. This flag is still needed for the handling consecutive update scenarios. We probably don't need the remote sync engine's start up step to move things back to InProcess false, but that optimization can be done outside of this PR.
General Checklist
Given When Then
inline code documentation and are named accordinglytestThing_condition_expectation()
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.